A fluent interface would be more like

variations = Variations()
variations.randomize()
          .attribute("gear")
          .at_xpath(".//motor[@name='a1']")
          .with_method(Variations.COEFFICIENT)
          .sampled_from(Variations.UNIFORM)
          .with_range(0.5, 1.5)

Ok, that's what @CatherineSue suggested but I thought it's a little verbose, although more understandable.
So, Variations contains a list of variation objects right?
And if that is the case, is the randomize() method used to create a new variation object?

well they contain some way of storing variations. a list of objects seems fine. randomize() create a new internal record of a variation.

fluent interfaces are verbose but easy to read. yes, you could use a very long list of arguments, but what if the relevant arguments change based on other arguments?

for instance, if i want a uniform distribution i need a range=(min,max) argument, but if I use a guassian i need a (mean, std) argument. it gets messy very quickly.

Got it. @ryanjulian, please review the new implementation. Also, me and @CatherineSue discussed the implementation of the wrapper for the randomized environment and work is in progress on that side as well.

Variation objects should be immutable (i.e. there are no setters)

Sorry, I just noticed this. There's a trade-off between having the fluent interface pattern and having immutable properties.
In order to set the members of the Variation object without setters, we have to pass their attributes in the constructor, and that breaks the interface pattern. Otherwise, without the setter decorators and with the members that correspond to each property not previously defined, we get this kind of errors:

Traceback (most recent call last):
  File "rllab/dynamics_randomization/trpo_swimmer.py", line 13, in <module>
    at_xpath(".//geom[@name='torso']").\
  File "/home/aigonzal/ivanWorkspace/rllab/rllab/dynamics_randomization/variation.py", line 114, in at_xpath
    self._list[-1].xpath = xpath
AttributeError: can't set attribute

Another way to do it is by setting the members of the class Variation directly in Variations, but that breaks the immutability of the property.
Any advice of how to achieve this is well appreciated.

I was trying to use a dictionary to store the variation then add it to Variation class at last. But I found that we need the v.elem=e in MujocoModelGenerator, this needs to call the elem setter in Variation. So I didn’t think of a way to implement immutable Variation after all.

For how to implement it, see builder pattern.

To solve the problem with v.elem = e, the solution is to keep a local cache of those elems in MujocoModelGenerator, rather than storing the data inside variations. That would be the proper way to do it--I just used the dict in my example as a shortcut.

Note that you can use the variation object directly as a key in your dictionary, i.e.

elem_cache = {}

# caching elems
for v in variations:
    # find elem, = e
    elem_cache[v] = e

# making models
for v in variations:
    e = elem_cache[v]

Thanks for the comment. Done

Change to from rllab.dynamics_randomization import VariationDistributions

Reuse this variable from mujoco_env.py

Add one NotImplementedError for the if statement of v.distribution as well.

Only set those variables that depend on self._wrapped_env.sim

What is this line for?

shouldn't be here. will delete in the new commit.

Alphabetical order c->d->e

m->n

Alphabetical order

hmm. if you return self then the API with_range() will be available after the dot operator.

e.g.

v.randomize()
 .attribute("gear")
 .at_xpath("something")
 .with_method(COEFFICIENT)
 .sampled_from(GAUSSIAN)
 .with_range(0.5, 1.5)

but .with_range() is not valid for .sampled_from(GAUSSIAN). you would want something like .with_parameters((1, 0.5)) or .with_mean_std(1, 0.5)

One way to solve this would be to return an instance of some new class without the method .with_range(), and then Python would throw an error at import time. That would be ideal but perhaps difficult to implement.

Another way is to change your API slightly to look like this

v.randomize()
 .attribute(gear)
 .at_xpath("something")
 .with_method(COEFFICIENT)
 .sampled_from(GAUSSIAN)
 .with_range(0.5, 1.5)
 .add()

Where add() is void (it doesn't return anything), and the new variation record is not constructed and added to the list of variations until add() is called, e.g.

class Variations():

    def randomize():
        return VariationSpec(self)


class Variation():

    def __init__(self, spec):
        # check a bunch of conditions on spec, raise an exception if it's wrong or incomplete or overspecified


class VariationSpec():
    
    def __init__(self, variations):
        self._variations = variations

    def with_xpath(xpath):
        self._xpath = xpath
        return self

    def add():
        self._variations.add(Variation(self))
        

if this turns out too much a pain to implement, on the other hand, it might be a sign that a fluent interface is not the right fit here

Done. Implemented the first approach by segmenting the Variations class into different classes, one base class, and two classes for Gaussian and uniform distributions.

To remove the setter in Variation, I changed the API using the second approach.

i think this should be a subpackage of rllab.envs.mujoco, as it's only relevant to MuJoCo environments.

Done. Should I replace the new rllab.env.mujoco import for all packages in rllab?

I'm not sure what you're asking. Can you clarify?

To address this issue, I added osp in the modules of the package rllab.env.mujoco, so now we can import osp as from rllab.envs.mujoco import osp.
I noticed other modules in rllab import osp as import os.path as osp. My question is if I should replace those imports withfrom rllab.envs.mujoco import osp.

Do you mean the dynamics_randomization package should be moved to rllab.envs.mujoco.dynamics_randomization?

This is a ValueError, not an AttributeError.

Your error messages would be more helpful if they told me which node could not be found in which file.

Done.

Despite leading to a KeyError, this is probably still a ValueError, not a KeyError.

This would be more helpful if it told me which attribute could not be found on which node in which file.

Done

Be careful to check for multiple results here.

This is an API question: what if an XPath matches multiples nodes
Should we:

1. Raise an error
2. Attempt to modify all matching nodes
3. Something else?

I vote for (1) but I'm interested in your opinion. The problem I have with (2) is that it is hard to tell the difference between matching multiple nodes on accident (due to a malformed XPath) versus someone legitimately wanting to address multiple nodes.

Working on it

I read the lxml document. The ElementTree library in lxml supports:findall() returns a list of matching Elements, find() efficiently returns only the first match. So if we use find(), there will be no error.

Great!

Usually when people format method chains onto multiple lines, they break before the .
e.g.

variations.randomize() \
          .attribute("gear")

Working on it.

You can just call this Method since its use is clear inside the package.

Done

You can just call this Distribution since its use is clear inside the package.

Done

It will be important to benchmark this to ensure threads (as opposed to processes) actually speed things up.

Python has a design detail called the GIL (global interpreter lock), which means that usually only one thread inside a Python application may run at once. This does not necessarily apply to parts of Python programs which are implemented using native code (e.g. mujoco), but whether that code is blocked by the GIL is very implementation-specific.

This is a great implementation, but I think you can make it even simpler by using the maxsize parameter of Queue

When maxsize != 0, put() blocks when the queue is full, until get() is called. By default, get() blocks when the queue is empty, until something is put() onto the queue.

I propose we make this a wrapper which looks a lot like normalized().

So we put these classes in rllab.envs.mujoco.dynamics_randomization. At the bottom of dynamics_randomization.py you can put

# dynamics_randomization.py
randomize_dynamics = RandomizedDynamicsEnv

So that users can use it in a launcher like this

# in your launcher
from rllab.envs.mujoco.dynamics_randomization import randomize_dynamics    # currently RandomizedEnv
from rllab.envs.mujoco.dynamics_randomization import Variations
from rllab.envs.mujoco.dynamics_randomization import Method    # currently VariationMethod
from rllab.envs.mujoco.dynamics_randomization import Distribution    # currently VariationDistribution

variations = Variations()
variations.randomize()\
          .at_xpath(".//geom[@name='torso']")\
          .attribute("density")\
          .with_method(Method.COEFFICIENT)\
          .sampled_from(Distribution.UNIFORM)\
          .with_range(0.5, 1.5)

env = randomize_dynamics(SwimmerEnv(), variations)

Done

Err, after trying the module I actually agree with some of your original naming. This module is general enough to randomize any part of an environment (good job!)

So the new vocabulary would be randomize = RandomizedEnv

This can be removed, it was only in my sample for testing purposes.

If you want to use it in a test, you should be able to find it directly in the mujoco_py package. Or you can use some other mujoco XML file already in our own tree (preferable).

You can also just define the XML inline in a string at the top of the file. That's probably the easiest option.

Generally you split new variations into different lines (e.g. no calling randomize twice in one statement). This is part of why I suggested that there be an add() method which is void to finish the statement.

See the builder pattern

For how to implement it, see builder pattern.

To solve the problem with v.elem = e, the solution is to keep a local cache of those elems in MujocoModelGenerator, rather than storing the data inside variations. That would be the proper way to do it--I just used the dict in my example as a shortcut.

Note that you can use the variation object directly as a key in your dictionary, i.e.

elem_cache = {}

# caching elems
for v in variations:
    # find elem, = e
    elem_cache[v] = e

# making models
for v in variations:
    e = elem_cache[v]

what stops me from using this method along with with_distribution()?

Two ideas to fix this. First one is to raise an error during the runtime. e.g.

variations = Variations()
variations.randomize() \
        .attribute("gear") \
        .at_xpath(".//motor[@name='a1']") \
        .with_method(Method.COEFFICIENT) \
        .sampled_from(Distribution.UNIFORM) \
        .with_mean_std(0.5, 1.5) \
        .add()

will raise

ValueError: Need to call with_range when sampled from Uniform

The current commit implements this idea.

Another idea is implement something like this

There is no with_mean_std when user calls sampled_from(UNIFORM)
I tried using mixin to achieve this but failed. Because using inheritance will return either VariationsGaussian or VariationsUniform in sampled_from. There's no way to tell which one should be called before running time.